KEP-5677: DRA Resource Availability Visibility#5749
KEP-5677: DRA Resource Availability Visibility#5749k8s-ci-robot merged 10 commits intokubernetes:masterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
ca95081 to
d9ac678
Compare
|
@nmn3m: GitHub didn't allow me to request PR reviews from the following users: kubernetes/sig-scheduling, kubernetes/sig-node, kubernetes/sig-cli, kubernetes/wg-device-management. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
d9ac678 to
495b6cb
Compare
|
/cc @johnbelamaric |
|
/cc @kubernetes/sig-cli-kubectl-maintainers |
|
/wg device-management |
keps/sig-scheduling/5677-dra-resource-availability-visibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-node/5677-dra-resource-availability-visibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-node/5677-dra-resource-availability-visibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-node/5677-dra-resource-availability-visibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-node/5677-dra-resource-availability-visibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-node/5677-dra-resource-availability-visibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-node/5677-dra-resource-availability-visibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-node/5677-dra-resource-availability-visibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-node/5677-dra-resource-availability-visibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-node/5677-dra-resource-availability-visibility/README.md
Outdated
Show resolved
Hide resolved
johnbelamaric
left a comment
There was a problem hiding this comment.
First pass, this is looking really really good to me so far
keps/sig-node/5677-dra-resource-availability-visibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-scheduling/5677-dra-resource-availability-visibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-node/5677-dra-resource-availability-visibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-node/5677-dra-resource-availability-visibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-node/5677-dra-resource-availability-visibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-node/5677-dra-resource-availability-visibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-scheduling/5677-dra-resource-availability-visibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-scheduling/5677-dra-resource-availability-visibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-node/5677-dra-resource-availability-visibility/README.md
Outdated
Show resolved
Hide resolved
keps/sig-node/5677-dra-resource-availability-visibility/README.md
Outdated
Show resolved
Hide resolved
495b6cb to
fdbf949
Compare
dd8b15d to
f26f289
Compare
Of those three options, the first seems the best to me as well. If it's an object we intend to be created, waited for, read, then deleted only to get a view of status, making it separate from a general We should also define behavior when multiple of these exist at the same time for the same pool (controller calculates once then fills all, etc). |
liggitt
left a comment
There was a problem hiding this comment.
some questions about the filtering / scale / limit bits, but those seem ok to pin down in implementation review as well
| | `driver` | Filter by driver name (optional) | | ||
| | `poolName` | Filter by pool name (optional, requires driver) | |
There was a problem hiding this comment.
making both of these optional means status is very unbounded ... should at least driver be required?
There was a problem hiding this comment.
I think, it will be no problem in that.
@johnbelamaric WDYT?
| |-------|-------------| | ||
| | `driver` | Filter by driver name (optional) | | ||
| | `poolName` | Filter by pool name (optional, requires driver) | | ||
| | `limit` | Max pools to return (default: 100, max: 1000) | |
There was a problem hiding this comment.
I expected a request structure that would not result in unbounded status that would require limits like this
There was a problem hiding this comment.
I'm also not sure where the 100 / 1000 limits came from ... with ResourceSlice, we've been really specific about the maximum size possible if all fields / lists are at their maximum size, to be sure the resulting resource could actually be persisted. Was that done here?
There was a problem hiding this comment.
No, the rigorous max-size calculation was not done. The 100/1000 numbers were chosen based on patterns in other K8s APIs, not from first principles. If we keep a limit field, we should do the proper calculation.
Alternatively, if we make driver required, the response becomes naturally bounded and the limit field may not be needed.
@liggitt WDYT?
There was a problem hiding this comment.
there could still be a LOT for a given driver. I think we should have a limit. If we can calculate that now that's great, but we can also defer it to implementation time.
There was a problem hiding this comment.
if we need a limit, make sure it is principled, and consider whether we need to make it user-specifiable, and consider whether the use cases we intend will break if a truncated response is received (e.g. an autoscaler couldn't use truncated info, right?)
…tate enum, add Security Considerations and Consistency Handling sections
Signed-off-by: Nour <nurmn3m@gmail.com>
Signed-off-by: Nour <nurmn3m@gmail.com>
…alidation Signed-off-by: Nour <nurmn3m@gmail.com>
Signed-off-by: Nour <nurmn3m@gmail.com>
…Freeze requirement and removed sig-scheduling Signed-off-by: Nour <nurmn3m@gmail.com>
|
/approve @johnbelamaric Please cancel the hold once ready. Thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kannon92, mrunalp, nmn3m The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…ased on size calculations Signed-off-by: Nour <nurmn3m@gmail.com>
f26f289 to
a3d1151
Compare
|
/hold cancel Thank you! |
|
/lgtm |
|
(post-merge note ... we might want to consider automatically deleting these via the controller after a fixed time period after creation / population ... we do that with other resources like certificate requests, etc ... can discuss during implementation) |
One-line PR description: Adding KEP-5677 for DRA Resource Availability Visibility
Issue link: DRA: Resource Availability Visibility #5677